Add Validation to Order Creation by Josiassejod1 · Pull Request #1038 · rubyforgood/stocks-in-the-future

Josiassejod1 · 2026-02-19T05:18:35Z

Pull Request

Summary

There isn't current validation on ExecuteOrder when there are insufficient funds or shares, thus, this PR adds some tests and checks.

Related Issue

Resolves #1010

Changes

Add a valid_order check in the ExecuteOrder service that leverages similar logic that already exists in the order model

Screenshots (if applicable)

Checklist

[X ] Issue is assigned (commenting on the issue page is needed)
[X ] Issue link added to the PR's description
[X ] Branch created from main
[ X] Commits are small and descriptive
[X ] Ran linter and fixed issues
[ X] Ran tests and all tests pass
[X ] CI checks passing
Review requested from team members

Notes

Discuss new or non-trivial changes in #stocks-in-the-future slack channel before or during implementation.

pollygee

Thanks @Josiassejod1 for your work here.
I don't think this current implementation is working to do the validations we need, so I've added some questions and comments. Check them out and let me know what you think If you have questions feel free to hit me up on slack!

Thanks again for your effort, we're on the right track and with some small updates this will be working just as needed.

pollygee · 2026-02-23T01:02:20Z

app/services/execute_order.rb

+      order.sufficient_shares?
+    end
+
+    true


because this is outside of the if block this function will always return true and that isn't what we want we'd want it to return the result of sufficient_funds or sufficient_shares.

good callout I need to call return

pollygee · 2026-02-23T01:25:10Z

app/models/order.rb

+    return true unless number_of_shares_valid_for_calculations?
+
+    current_shares = user.portfolio&.shares_owned(stock_id) || 0
+    shares <= current_shares


Same here, we'll need to add an error if there aren't enough shares not just return false so we have an indication of why it didn't work.

This makes sense to me!

pollygee · 2026-02-23T01:25:12Z

app/models/order.rb

+
+    current_balance_cents = (user.portfolio&.cash_balance || 0) * 100
+
+    fee_already_subtracted = user.orders.pending.exists? ? PortfolioTransaction::TRANSACTION_FEE_CENTS : 0


I think the purpose of this function here here is not calculating "if" the fee was already calculated but rather what fee we need to add

maybe a better name could be fee_to_charge

pollygee · 2026-02-23T01:25:19Z

test/services/execute_order_test.rb

+      order.save!
+    end
+
+    assert order.errors[:shares].first.include?("Insufficient funds")


Same question as below, is this the error message we will get?

pollygee · 2026-02-23T01:25:22Z

test/services/execute_order_test.rb

+      order.save!
+    end
+
+    assert order.errors[:base].first.include?("Cannot sell more shares than you own")


Is this the error message we'd get I don't see it added as an error when we do the check.

pollygee · 2026-02-23T01:26:40Z

app/models/order.rb

+
+    total_needed = purchase_cost + transaction_fee
+
+    balance_without_reservations >= total_needed


We need to add an error here to alert the user to why the transaction fails if it does fail.
Can you add something like:
errors.add(:shares, "Insufficient funds. You have #{formatted_balance} but need #{formatted_cost}")?

Josiassejod1 · 2026-02-23T22:37:14Z

Thanks for the clarity !

pollygee · 2026-02-25T14:35:45Z

@AbuMareBear do you have any availability any time soon to take a look at these updates to the PR?

pollygee

Thanks for your work on this I think we're closer.
I still have some concerns here. We do already have validations on order for when an order is created.

This order execution happens in a job that runs at night so it isn't user facing. I think we might be able to reuse some of that work that has been done to validate enough stocks and cash to buy?

Feel free to ping me in slack if you want to talk through this a little more.

pollygee · 2026-02-27T01:51:39Z

app/models/order.rb

+    current_balance_cents = (user.portfolio&.cash_balance || 0) * 100
+
+    fee_to_charge = user.orders.pending.exists? ? PortfolioTransaction::TRANSACTION_FEE_CENTS : 0
+    balance_without_reservations = current_balance_cents + purchase_cost + fee_to_charge


I'm confused with this line here, I'm not sure what this is calculating. How is the balance something we're getting from adding the purchase cose and fees to be charged?

pollygee · 2026-02-27T01:57:35Z

app/models/order.rb

+
+    return true if shares <= current_shares
+
+    formatted_shares = (current_shares % 1).zero? ? current_shares.to_i : current_shares


I don't think we need this formatting because this is being run by a job so the user will not see an error message here at all.

Josiassejod1 added 2 commits February 19, 2026 00:15

Add Validation to Order Creation

da266a3

lint

9615d13

Josiassejod1 marked this pull request as ready for review February 19, 2026 05:39

Josiassejod1 requested review from AbuMareBear and pollygee February 19, 2026 05:39

pollygee requested changes Feb 23, 2026

View reviewed changes

Josiassejod1 added 4 commits February 23, 2026 22:40

updated

338af91

inline method calls

327dd0f

Update order.rb

947a8a0

Merge branch 'main' into dalvin/add-validation-to-execute-order

90cec25

Josiassejod1 requested a review from pollygee February 25, 2026 06:39

pollygee requested changes Feb 27, 2026

View reviewed changes


		current_balance_cents = (user.portfolio&.cash_balance \|\| 0) * 100

		fee_already_subtracted = user.orders.pending.exists? ? PortfolioTransaction::TRANSACTION_FEE_CENTS : 0


		total_needed = purchase_cost + transaction_fee

		balance_without_reservations >= total_needed


		return true if shares <= current_shares

		formatted_shares = (current_shares % 1).zero? ? current_shares.to_i : current_shares

Conversation

Josiassejod1 commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Summary

Related Issue

Changes

Screenshots (if applicable)

Checklist

Notes

Uh oh!

pollygee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Josiassejod1 commented Feb 23, 2026

Uh oh!

pollygee commented Feb 25, 2026

Uh oh!

pollygee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Josiassejod1 commented Feb 19, 2026 •

edited

Loading